feat(bucketpolicy): add the bucket policy and its evaluation by pyropy · Pull Request #60 · fil-forge/hilt

pyropy · 2026-09-10T16:30:33Z

Adds pkg/bucketpolicy, the pure policy document package: validation, evaluation, and a canonical form whose lists are sets. The ETag is the CID of the document's DAG-CBOR encoding. Field names and effects follow AWS: Statement, Sid, Effect, Principal, Action, Allow, Deny.

Decode, Validate, Canonical, ETag
Effective (Allow minus Deny), Named, Changed
Hand-written DAG-CBOR encoder; table tests

Change log

d2a610d: AWS field names and capitalization; lowercase keys are refused.

References

RFC: rfc: forge S3 tenant IAM fil-one/RFC#30 (commit 113d4ded: AWS field names)

🤖 Generated with Claude Code

Copilot

🟡 Changes recommended

Exclude * from queried principals in Effective and add the corresponding test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a pure bucket-policy package for validation, canonical ETags, permission evaluation, and change detection.

Changes:

Defines policy documents, statements, effects, and validation rules.
Implements canonical JSON, SHA-256 ETags, effective permissions, and change tracking.
Adds table-driven tests.

File summaries

File	Summary
`pkg/policy/policy.go`	Implements policy validation, canonicalization, evaluation, and change detection. Moderate issue (2 votes): `Effective` treats `*` as a queryable principal.
`pkg/policy/policy_test.go`	Tests policy validation, canonicalization, ETags, evaluation, and change detection.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

pyropy · 2026-09-16T15:15:18Z

@codex review

chatgpt-codex-connector · 2026-09-16T15:15:37Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-28T16:55:03.826051Z	`d2aa4d1`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1133cac89e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

alanshaw · 2026-10-05T11:49:42Z

+	if err != nil {
+		panic(err) // sha2-256 with the default length cannot fail
+	}
+	return `"` + cid.NewCidV1(cid.DagCBOR, sum).String() + `"`


Yeah... I think if you defined the policy types something like I suggested fil-one/RFC#30 (comment) you could use dag-json-gen to deterministically encode the policy.

Then you lose the normalize, sortedSet, strictKeys and encodeCBOR functions since you get all that for free just by using the format.

So you'd Sum over policy.MarshalDagJson() here and use the DAG JSON IPLD codec in the CID.

I don't think this has to block the PR but would have to go in before prod if we do do it.

Should be fixed in 7d8a79a

Adds pkg/bucketpolicy, a pure package with no store or transport dependencies. Validate applies the RFC's 422 rules: at least one statement, a recognized effect, non-empty principals and actions, actions from the policy vocabulary (s3perm.PolicyAction), and named principals that exist for the tenant. Canonical produces compact JSON in a fixed field order with input order kept, and ETag is the quoted hex SHA-256 of that encoding, so stored tags compare with If-Match values. Effective computes a principal's actions on a bucket: the union of Allow statements naming it or the wildcard, minus the union of Deny statements naming it or the wildcard. Named lists the principals a policy names and whether it uses the wildcard. Changed lists the principals whose effective set differs between two policies, expanding the wildcard to the tenant's principals, which is the set a policy write must invalidate. The wildcard is not itself a principal: Effective is nil for "*", so only Changed expands it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

A principal list naming an id twice decoded as given and was stored that way. Decoding now keeps the first occurrence of each id, so every write path that goes through Decode, the policy API and the CreateBucket header alike, stores and tags the filtered document. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

…r CID The statement, principal and action lists are sets: Decode and Canonical deduplicate and sort them, and Hilt stores and returns the sorted document. The ETag is the CID (v1, dag-cbor, sha2-256) of the DAG-CBOR encoding of that document, so two documents that differ only in order or in duplicates carry the same tag. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

encoding/json decodes "sid": null into an empty string without error, so the policy was accepted and stored without its sid. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The bucket policy JSON uses the AWS names Statement, Sid, Effect, Principal and Action, and the effects Allow and Deny (RFC 30, 113d4ded). The strict key check keeps refusing any other spelling, so the lowercase names are now unknown fields, and Version and Resource stay refused. The DAG-CBOR key order is unchanged (length, then bytes), so the ETags change only through the renamed keys and effects. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Policy.UnmarshalJSON runs the strict decode, the structural checks and the normalization, so a policy that is not well formed never comes into existence: not from Decode, and not from a store reading a document back with encoding/json. Validate keeps the principal-existence check and repeats the structural one for documents built in code, which the service's write path still passes through it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The ETag is the CID (CIDv1, dag-json, sha2-256) of the policy encoded as sets, following the schema from the IAM RFC: each statement's principal and action lists are maps from member to {}, the wildcard principal is the set {"*"}, and the policy is the set of its statements' CIDs. The encoder is generated with dag-json-gen (pkg/bucketpolicy/internal/dagjson, go generate), which writes map keys and struct fields sorted, so the order the caller wrote things in and any duplicate cannot reach the bytes. The canonical AWS-shaped document is derived from the same form: statements ordered by CID, principals and actions sorted. That replaces the hand-built DAG-CBOR encoding and the sort by JSON encoding. The encoded types live in their own import-free package because the generator builds with -tags codegen, which excludes libforge's generated files and so anything that imports s3perm. The generator's map limit becomes a parse-time bound of 4096 statements and principals per statement, so a document that parses always encodes. strictKeys stays: the request body is the AWS array shape with a Principal union, which the generator cannot express, and encoding/json matches names case-insensitively. The two pinned ETags are re-pinned from hand-assembled DAG-JSON bytes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at ccf4093. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38577102150. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)

Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at 8f6b78d. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38581685131. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)

Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at d236fcd. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38583928528. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)

Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at d236fcd. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38583928528. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070) Co-authored-by: fil-forge-bot[bot] <318653112+fil-forge-bot[bot]@users.noreply.github.com>

Sets every prod pin in `terraform/envs/prod/apps/terraform.tfvars` to what `terraform/envs/staging/apps/terraform.tfvars` pins at 6cdeb78. Merging this is the promotion: Check and deploy applies `terraform/envs/prod/apps` on the merge commit, with no further confirmation. staging passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38589211473. This pull request is rebuilt each time staging passes its smoke test after a push to `main`, and closed once prod matches staging. To hold a service back, push to `bot/promote-prod` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over prod's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)

pyropy added this pull request to stack #62 September 10, 2026 16:30

pyropy force-pushed the srdjan/feat/iam-policy-package branch from d4024b1 to f797e0f Compare September 11, 2026 10:37

pyropy removed this pull request from stack #62 September 11, 2026 10:38

pyropy force-pushed the srdjan/feat/iam-policy-package branch from f797e0f to 75af1ea Compare September 11, 2026 10:59

pyropy added this pull request to stack #71 September 11, 2026 10:59

pyropy requested a lite review from Copilot September 11, 2026 12:29

Copilot started reviewing on behalf of pyropy September 11, 2026 12:30

View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread pkg/bucketpolicy/bucketpolicy.go Outdated

pyropy force-pushed the srdjan/feat/iam-policy-package branch from 75af1ea to 8c144d5 Compare September 11, 2026 12:46

pyropy changed the title ~~feat(policy): add the bucket policy document and its evaluation~~ feat(bucketpolicy): add the bucket policy and its evaluation Sep 11, 2026

pyropy force-pushed the srdjan/feat/iam-policy-package branch 2 times, most recently from 2821817 to 7ecf429 Compare September 16, 2026 14:11

pyropy force-pushed the srdjan/feat/iam-policy-package branch from 7ecf429 to 1133cac Compare September 16, 2026 14:49

pyropy requested a lite review from Copilot September 16, 2026 15:10

Copilot AI reviewed Sep 16, 2026

chatgpt-codex-connector Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread pkg/bucketpolicy/bucketpolicy.go Outdated

pyropy force-pushed the srdjan/feat/iam-policy-package branch from 1133cac to 6c425fc Compare September 17, 2026 13:34

pyropy force-pushed the srdjan/feat/iam-policy-package branch 2 times, most recently from a981176 to fac3ea6 Compare September 22, 2026 14:17

pyropy force-pushed the srdjan/feat/iam-policy-package branch from fac3ea6 to 6dc5b91 Compare September 23, 2026 10:29

pyropy force-pushed the srdjan/feat/iam-policy-package branch from 362147b to dfdada0 Compare September 30, 2026 16:23

pyropy force-pushed the srdjan/feat/iam-policy-package branch 2 times, most recently from 660aaa0 to b063707 Compare September 30, 2026 16:58

pyropy marked this pull request as ready for review September 30, 2026 17:17

pyropy force-pushed the srdjan/feat/iam-policy-package branch from a7fdfdf to 7edd92e Compare October 5, 2026 09:40

alanshaw approved these changes Oct 5, 2026

View reviewed changes

pyropy force-pushed the srdjan/feat/iam-policy-package branch from d2a610d to 7d8a79a Compare October 5, 2026 14:22

pyropy and others added 7 commits October 5, 2026 16:56

fix(bucketpolicy): reject a null sid

03281c3

encoding/json decodes "sid": null into an empty string without error, so the policy was accepted and stored without its sid. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pyropy force-pushed the srdjan/feat/iam-policy-package branch from 7d8a79a to b4c7f06 Compare October 5, 2026 15:08

pyropy removed this pull request from stack #86 October 9, 2026 07:33

pyropy added this pull request to stack #95 October 9, 2026 07:33

pyropy merged commit b30465e into main Oct 14, 2026
12 of 19 checks passed

pyropy deleted the srdjan/feat/iam-policy-package branch October 14, 2026 11:21

fil-forge-bot Bot mentioned this pull request Oct 14, 2026

Promote dev's images to staging fil-forge/infra-central#232

Merged

This was referenced Oct 14, 2026

Promote dev's images to staging fil-forge/infra-central#242

Closed

Promote staging's images to prod fil-forge/infra-central#243

Merged

Navigation Menu

Repository navigation

feat(bucketpolicy): add the bucket policy and its evaluation - #60

feat(bucketpolicy): add the bucket policy and its evaluation#60
pyropy merged 7 commits into
srdjan/feat/iam-principal-storefrom
srdjan/feat/iam-policy-package

pyropy commented Sep 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

pyropy commented Sep 16, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

alanshaw Oct 5, 2026

Uh oh!

pyropy Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

pyropy commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change log

References

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

pyropy commented Sep 16, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

alanshaw Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

pyropy Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pyropy commented Sep 10, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 16, 2026 •

edited

Loading